Code review

Found 8 issues:

1. [CRITICAL:100] Cross-channel approval thread hijacking - authorization bypass
   An attacker can approve/deny tool requests on threads they didn't initiate by supplying the target thread UUID to the approval endpoint. The code at line 1039 only checks if a thread exists in the session without verifying channel authorization context or ownership of the approval request. This allows potential escalation of privileges across channels.

https://github.com/anthropics/ironclaw/blob/067ff01a2780fd91354b7902b4743316293beba0/src/agent/agent_loop.rs#L1033-L1051

2. [CRITICAL:95] TOCTOU race condition in approval thread resolution
   The code checks thread existence and mutates session state (lines 1040-1041), then drops the lock before completing the async register_thread() call (lines 1042-1050). This violates the session locking invariant documented in src/agent/CLAUDE.md:127 and creates a window where concurrent operations could modify session state after the mutations but before the thread is registered. The entire check-mutate-register sequence must complete while holding the write lock.

https://github.com/anthropics/ironclaw/blob/067ff01a2780fd91354b7902b4743316293beba0/src/agent/agent_loop.rs#L1038-L1051

3. [HIGH:70] Incomplete fallback logic for non-existent approval threads
   When an approval targets a UUID that doesn't exist in the local session, the code falls back to resolve_thread() with the same UUID string (line 1053). But resolve_thread() expects either None or a conversation_scope value, not a UUID. This fallback may not correctly handle or create the intended thread.

https://github.com/anthropics/ironclaw/blob/067ff01a2780fd91354b7902b4743316293beba0/src/agent/agent_loop.rs#L1052-L1062

4. [MEDIUM:85] Unnecessary repeated UUID parsing in hot path
   The approval thread UUID is parsed from string on every approval submission via Uuid::parse_str(thread_id).ok() (lines 1022-1031). For high-frequency approval workloads, this repeated parsing could accumulate. Consider caching the parsed UUID or parsing once during message deserialization.

https://github.com/anthropics/ironclaw/blob/067ff01a2780fd91354b7902b4743316293beba0/src/agent/agent_loop.rs#L1022-L1031

5. [MEDIUM:80] Lock contention in approval thread resolution
   Multiple sequential lock acquisitions: get_or_create_session() write lock, session.lock(), then register_thread() with additional locks (lines 1036-1050). The explicit drop(sess) hints at intentional lock management but suggests the lock pattern needs review. Under high approval load, this sequential locking could cause contention.

https://github.com/anthropics/ironclaw/blob/067ff01a2780fd91354b7902b4743316293beba0/src/agent/agent_loop.rs#L1036-L1050

6. [MEDIUM:70] Missing architectural trait for approval submission routing
   The code hardcodes a type check for Submission::ExecApproval { .. } | Submission::ApprovalResponse { .. } to detect approval submissions that can bypass normal thread resolution (lines 1022-1031). Per CLAUDE.md architecture guidelines, trait-based design is preferred over hardcoding conditionals. A method like submission.can_target_existing_thread() would make the intent explicit.

https://github.com/anthropics/ironclaw/blob/067ff01a2780fd91354b7902b4743316293beba0/src/agent/agent_loop.rs#L1022-L1031

7. [MEDIUM:75] Test URLs changed to use public IPs
   Test URLs in config tests changed from https://custom.example.com to https://8.8.8.8 and similar. While these are test fixtures, using public IP addresses (Google DNS) in test mocking could accidentally expose real network calls if test isolation fails. Use RFC 5737 TEST-NET addresses (192.0.2.x) or localhost instead.

https://github.com/anthropics/ironclaw/blob/067ff01a2780fd91354b7902b4743316293beba0/src/config/embeddings.rs#L302
https://github.com/anthropics/ironclaw/blob/067ff01a2780fd91354b7902b4743316293beba0/src/config/llm.rs#L858

8. [LOW:70] Unnecessary UTC time fetch on every approval thread reuse
   Line 1041 calls chrono::Utc::now() every time an existing approval thread is reused. This syscall is unnecessary for functionality and could accumulate under high approval frequency. Consider deferring to periodic heartbeat updates.

https://github.com/anthropics/ironclaw/blob/067ff01a2780fd91354b7902b4743316293beba0/src/agent/agent_loop.rs#L1040-L1041